Repository navigation
Remove dead code from bun init, bun create, bun_md, bun_install, and the socket and Postgres code - #44102
Remove dead code from bun init, bun create, bun_md, bun_install, and the socket and Postgres code#44102robobun wants to merge 1 commit into
Conversation
…the socket and Postgres code Each group cannot run, or holds a value that nothing reads. No lint reports them, because each still has a textual user: an unreachable call, a store, or a parameter that every caller sets to one value. - bun init: the React dependency tables and the code that wrote a `dependencies` object. `exec` returns early for the React templates. - bun create: `CreateListExamplesCommand` (the help text exits first), the `Option` of `Example::print`, the progress node parameter of `fetch_all_local_and_remote` and `fetch_all`, and three constant locals with the hints they guarded. - bun_md: the first ATX trim block in `analyze_line` (`line.end` is 0 there) and two branches of `is_container_compatible` that do not change its result. - bun_install: `Subcommand::Pack` and `PACK_PARAMS`. `bun pm pack` runs as `Subcommand::Pm`. - Sockets: `Flags::OWNED_PROTOS`, which only guarded a store of `None` right before a store of the new value. - Postgres: the map `backend_parameters`, which nothing reads. - bun_ast, bun_js_parser: `Ast.runtime_imports`, which nothing reads. - `src/jsc/bindings/JSCInlines.h`, which no include reaches.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe changes remove package-manager and parser state, CLI template behaviors, a JavaScriptCore header, socket protocol-flag handling, and PostgreSQL parameter storage. They also adjust Markdown container compatibility and ATX header trimming. ChangesAST parser state
Package manager Pack subcommand
JavaScriptCore bindings header
Markdown parsing
CLI template commands
Socket protocol flags
PostgreSQL backend parameters
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The documented pack command remains available, and no behavior change requiring a fix was established. The PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. CI is green. This PR only deletes code, so it has no new test. CI on commit ee4ef39, which is on the current head of main (a4f1429):
How it was checked before the push:
This PR conflicts with four open PRs, which need a rebase after the merge: #41598, #43942, #38474, #39701. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it deletes 550 lines across ten unrelated subsystems (sockets/TLS, Postgres, the parser, the markdown parser, three CLI commands) and some of the removed code was a latent product surface (the bun create examples listing, Postgres backend_parameters), a maintainer should still confirm these are wanted deletions rather than features to revive.
What was reviewed:
- Independently confirmed each deadness proof: React templates return at init_command.rs:618 before the dependency tables;
bun_createin cli/mod.rs exits on missing positionals beforeCreateCommand::exec;JsCell::setalready drops the previousprotosBox so theOWNED_PROTOSguard was a no-op;line.endis 0 fromLine::default()until blocks.rs:564, so the removed ATX trim block never ran; the oldis_container_compatibletruth table equalsexisting.ch == new.ch. - Grepped for surviving references: no reader of
Ast.runtime_importsorbackend_parameters, no constructor ofSubcommand::Pack(enum derives onlyIntoStaticStr, no integer casts), no unprefixed"JSCInlines.h"include anywhere in src/ or cmake/. - Checked all remaining
SocketFlags::default()users differ only in the removed bit, andExample::print/fetch_all*callers all match the new signatures.
Extended reasoning...
The change is a single commit removing dead code from 13 files (+20/-550) spanning the JS parser AST, package-manager CLI arg parsing, a vendored JSC umbrella header, the markdown block/container parser, bun init/bun create, the TLS socket ALPN protos ownership flag, and Postgres ParameterStatus handling. It touches the TLS socket and Postgres wire-parsing surfaces, but the edits there only remove a redundant drop guard and an unread map; no authentication, validation, or crypto logic changes. Every deletion I checked is provably unreachable or unread at HEAD, no CODEOWNERS entry covers the changed paths, and the bug hunt ran dry. The breadth across unrelated subsystems and the fact that two removed groups were dormant user-facing features (examples listing, backend parameter capture) are what tip this to defer rather than approve.
|
History of the two groups that the review names as possible features:
Both facts are now in the Notes of the description. If a maintainer wants to keep one of these groups, I will remove it from this PR. |
|
@robobun wake up!! |
|
@robobun wake up!! |
|
I am here. The state of this PR:
The PR waits for your review. If you want a change, tell me which one. For example, I can remove one group from the PR. |
) Behaviour change: none One error path differs. See Downsides. ### Problem - `bun build --dump-environment-variables` does nothing. `BUILD_ONLY_PARAMS` declares the flag (`src/runtime/cli/Arguments.rs:538`), but no code reads it. - `DebugOptions.dump_environment_variables` is always `false`, so the branch at `src/runtime/cli/build_command.rs:604` and `Transpiler::dump_environment_variables` cannot run. - The completions still offer the flag, with `--dump-limits` and `--disable-bun-js`, whose fields #36184 deleted. ### Fix - Delete the param, the field, the branch and the function. Remove the three names from the completions. - Correct on current main: the parser skips an unknown long flag in silence (`src/clap/streaming.rs:157`). The bare flag gives the same bundle as before. - Verified: `bun bd`, `test/bundler/cli.test.ts`, `bundler_env.test.ts`, `test/cli/bun.test.ts`, `test/cli/run/env.test.ts`. No new test: `REVIEW.md` says "Do not add tests to check dead code stays dead". - Self-reviewed: 14 concerns raised, 13 addressed. Rejected: to drop the first line. It stays because the PR adds no test. ### Background - The flag printed the environment map as JSON and stopped. `bun build` printed valid JSON up to bun 0.8.0, a malformed stream up to 1.0.11, and nothing since 1.0.12 (#6395). - #43745, #44102 and #44343 ask to connect the flag or remove it. Close this PR if a maintainer wants it connected. - Considered connecting it: it prints every process variable, and no `.env` value without `--env`. The docs give `bun --print process.env`. ### Downsides - `bun build --dump-environment-variables=value` exited 1 with "does not take a value". It is now skipped. - The bare flag still builds with no message. With #40560 (open) it exits 1, like every unknown flag. <details><summary>Notes</summary> #### Deleted, one line each - `parse_param!("--dump-environment-variables")` in `BUILD_ONLY_PARAMS` (`src/runtime/cli/Arguments.rs`). - `DebugOptions.dump_environment_variables` and its default (`src/options_types/context.rs`). - The `if ctx.debug.dump_environment_variables { ... }` branch in `BuildCommand::exec`, and the stale comment above it (`src/runtime/cli/build_command.rs`). - `Transpiler::dump_environment_variables` (`src/bundler/transpiler.rs`). `write_json_string` and the iterator of `bun_dotenv::Map` have other callers. - `completions/bun.zsh`: the `--dump-environment-variables` and `--dump-limits` specs in `_bun_run_completion`. - `completions/bun.bash`: `--dump-environment-variables --dump-limits --disable-bun-js` in `GLOBAL_OPTIONS`. #### Behaviour, measured Released bun 1.4.3 against a debug build of this branch. "Same bundle" means the same bytes on stdout as `bun build x.js`. | Input | Before | After | |---|---|---| | `bun build --dump-environment-variables x.js` | exit 0, same bundle | exit 0, same bundle | | `bun build --dump-environment-variables=1 x.js` | exit 1, "The argument '--dump-environment-variables' does not take a value." | exit 0, same bundle | | `bun build --dump-environment-variables= x.js` | exit 1, same error | exit 0, same bundle | | `BUN_OPTIONS=--dump-environment-variables=1 bun build x.js` | exit 1, same error | exit 0, same bundle | | `bun build --no-such-flag=1 x.js` | exit 0, same bundle | exit 0, same bundle | `bun build --help` never listed the flag. `WARN_ON_UNRECOGNIZED_FLAG` (`src/clap/streaming.rs:10`) is never set to true, so the skip prints nothing. This PR does not touch it. #40560 removes it. #### History - `38f83c50c4` and `5691bf385b` (October 2021) added the flag for the old `bun bun` and `bun dev` commands. - Runs of release binaries with `bun build --dump-environment-variables x.js`: 0.8.0 prints valid JSON. 1.0.1 and 1.0.11 print one JSON string per fragment (`"{",` then `"\n ",` then `"PATH",`), which is not valid JSON. The cause is `19aa9d93de` (#4233, first in 0.8.1): `Map.jsonStringify` changed from `writer.writeAll` to `writer.write` on the JSON stream. - Up to 1.0.11 the flag was declared one time, with help text, in `debug_params`, and `src/cli.zig:593` read it for every command. #6395 deleted that list and added a new declaration, with no help text, to `build_only_params` only. - In the branch of #6395, `5bf8ad8fb9` commented out the line that reads the flag, under a source comment that says the two dump flags were "only used in bun dev". `fbbb5e7e38` ("Remove comments") deleted the commented lines. The merged commit does not contain that comment. `bun build` had its own consumer (`src/cli/build_command.zig:220`), and it stayed. - #5712 is the only issue that names the flag. It asked for `bun run` on bun 1.0.2, and it ended with "This flag appears to have been removed". - No maintainer has ruled on the flag. #36184 deleted `DebugOptions.dump_limits` and `DebugOptions.fallback_only`, the fields of the other two debug flags. #41699 removed the dead Solid JSX runtime in the same way. #### If the flag is wanted back The one-line reader is not enough. `bun build --app` returns into `bake::production::build_command` before the branch. The branch runs after the compile target lookup, so it must move to directly after `configure_defines()`. `--watch` needs a decision. The flag needs help text and docs. #### Completions: what stays - The three names are the completion entries of the three old `DebugOptions` debug flags. After this PR none of their fields exists. - `completions/bun.bash` `GLOBAL_OPTIONS` still has nine names that no param table declares: `--use --bunfile --server-bunfile --disable-react-fast-refresh --disable-hmr --jsx-production --platform --public-dir --inject`. #35443 generates that file from `bun-cli.json`. #42275 rewrites the same line. - `_bun_run_completion` in `completions/bun.zsh` still has eleven specs that `bun run` does not declare: `--no-summary --version -v --revision --external --packages --minify --minify-syntax --minify-whitespace --minify-identifiers --target`. Other commands declare them. #### `DebugOptions` fields with no reader that stay - `output_file`: #44050 removes it. - `editor`: #38059 connects it. - `package_bundle_map`: `[bundle.packages]` in `bunfig.toml` fills it (`src/bunfig/bunfig.rs:910`) and nothing reads it. No open PR covers it. #### Other params with no reader 207 long flags are declared in `Arguments.rs`. 13 have no `b"--name"` reader in `src/**/*.rs`: - `--tls-min-v1.0`, `--tls-min-v1.1`, `--tls-min-v1.2`, `--tls-max-v1.3`: `src/js/node/tls.ts` reads them from `process.execArgv`. - `--trace-events-enabled` and six more `--trace-*` flags: declared on purpose, see the comment above them. - `--grep`: an alias of `--test-name-pattern`. - `--dump-environment-variables`: the only one with no reader and no stated reason. #### Checks - `test/bundler/cli.test.ts`: 38 pass, 0 fail. `test/bundler/bundler_env.test.ts`: 7 pass, 0 fail. `test/cli/bun.test.ts`: 39 pass, 0 fail. `test/cli/run/env.test.ts`: 107 pass, 0 fail. All with `--timeout 120000`, because the machine was under heavy load. - `bash -n completions/bun.bash` passes. zsh was not available, so `completions/bun.zsh` was checked by reading. The new last line of the `_arguments` call ends with `' &&`, like the other calls in the file. - The debug binary has no `dump-environment-variables` string. The completion scripts are embedded with `include_bytes!`, so `bun completions` installs the edited scripts. - Release binary size in CI against `main`: no target grows. Four of the twelve targets are 2 to 4 KB smaller (FreeBSD x64 and aarch64, Windows x64 and aarch64). - Substitutes, run on bun 1.4.3: `bun --print process.env` prints the process variables and the `.env` values. `logLevel = "debug"` in `bunfig.toml` makes `bun build --env inline` print the `.env` files that it loads. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check <!-- robobun:evidence:end -->
Behaviour change: none
Problem
bun initreturns early for every React template (src/runtime/cli/init_command.rs:615), so the React dependency tables after that return never run.Fix
bun bd,cargo checkfor 12 targets and each custom cfg, and the tests of each touched area.match templateininit_command.rs, which re-indents 88 lines in a file that 17 open PRs touch.Background
dead_code. But rustc counts a store to a field, and a call in a branch that cannot run, as uses.Ast.runtime_imports(Remove dead code from js_printer, bundler, js_parser, patch, shell, http #35978),Subcommand::Pack(Remove dead code: write-only fields and never-set options in 11 crates #43745),JSCInlines.h(Remove dead code from sys/windows, cli, shell, test_runner, v8 shim #36237, closed as stale).Downsides
bun createandbun pm packprint the same bytes as the released bun.Notes
Removed, one group each (net lines)
src/runtime/cli/init_command.rs(254):DependencyGroupwithREACT,TAILWIND,SHADCN,Template::{dependencies, is_react, write_to_package_json, scripts}, and the code that wrote adependenciesobject.execreturns inmatch templatefor the three React variants and does not assigntemplateafter that. So onlyBlankandTypescriptLibraryreach the later code, andBLANK.dependencieswas the empty slice. The React templates take their dependencies from their embeddedpackage.json. The one list that is left is nowconst DEV_DEPENDENCIES.src/runtime/cli/create_command.rs(86):CreateListExamplesCommand.bun_createexits with the help text when there is no positional, andCreateCommand::extract_infodoes the same, both beforeCreateCommand::exectestspositionals.is_empty(). With it gone, both callers ofExample::printpass a name and both callers offetch_all_local_and_remotepass no progress node. SoExample::printtakes&[u8], the staticAPP_NAME_BUFgoes, andfetch_all_local_and_remoteandfetch_alllose the node parameter.fetch_allonly stored that node inclient.progress_node, whose initial value isNone(src/http/AsyncHTTP.rs:174).bun createwith no template prints a usage text since Progress for Next.js #4468 (2023-09-07), which madebun create <template>runbunx create-<template>.src/runtime/cli/create_command.rs(14):is_nextjs,is_create_react_app,create_react_app_entry_point_path. Immutable locals bound tofalse,falseandb"", with theifthat printedbun bunhints.src/jsc/bindings/JSCInlines.h(57, whole file). All 147 includes in the tree nameJavaScriptCore/JSCInlines.h, andsrc/jsc/bindingshas noJavaScriptCoredirectory. WebKit headers that include"JSCInlines.h"with no prefix reach WebKit's file, because a quoted include searches the directory of the including file first. Bun's file was a stale variant: it includedOperations.hwhere WebKit's header includesOperationsInlines.h. Remove dead code from sys/windows, cli, shell, test_runner, v8 shim #36237 deleted the same file and its C++ compiled on every CI lane (build 87733).src/md/blocks.rs(26): the first ATX trim block inanalyze_line.*line = Line::default()setsline.endto 0, and the first store toline.endis after the loop. So both guards (line.end > line.beg) are false. The block afterline.end = offdoes the trimming.src/md/containers.rs,src/md/autolinks.rs(16):is_container_compatibleis nowexisting.ch == new.ch. One branch returnedfalseright before the function returnedfalse. Another tested for two>right before the test for equal characters. The helperis_list_bullethad no other user.src/runtime/socket/socket_body.rs,Listener.rs(15):Flags::OWNED_PROTOS. Its only readers guardedprotos.set(None)right beforeprotos.set(new). Its writers were theDefaultimpl and the rawupgradeTLSsocket.Flagsnow derivesDefault, which is the empty set. No code reads the flags word as a whole.src/sql_jsc/postgres/PostgresSQLConnection.rs(12): the mapbackend_parameters. TheParameterStatusmessage is still decoded, so the stream advances. TheOutOfMemoryerror of the insert is gone with the insert. The map has had no reader since the first Postgres client added it in Add unusable postgres client behind a canary-or-debug-only feature flag #11920 (2024-07-07).src/install/PackageManager/CommandLineArguments.rs,PackageManager.rs(41):Subcommand::PackandPACK_PARAMS.bun pm packruns asSubcommand::Pm. Nothing castsSubcommandto an integer.bun pm --helplists the pack flags.src/ast/ast_result.rs,src/js_parser/p.rs(9):Ast.runtime_imports. Written into_ast, never read. The parser keeps its own table.Verification
All on the final commit, rebased on a4f1429.
bun bdpasses. That is a full C++ compile against the new WebKit, withoutJSCInlines.h.cargo check --workspacepasses for 12 targets (scripts/rust-check-all.ts), with--all-targets, and on the host with--cfg=bun_codegen_embedand with--cfg=socket_fault_injection --cfg=bun_debug --cfg=bun_asan.cargo clippy,cargo fmt --checkandbun run rust:mordant(3 targets, fresh records) exit 0.test/cli/init/init.test.ts(17, asserts the dependencies of all three React templates),test/js/bun/md/(1098),bun-pack.test.ts,bun-pm.test.tsandbun-create.test.ts(129),transpiler.test.js,runtime-transpiler.test.tsandbundler_cjs2esm.test.ts(365),socket-retention.test.ts(7).postgres-*.test.ts,sql.test.tsand 3 more) with a 120 s timeout: 260 pass, 0 fail. An earlier run with the default timeout of 5 s had 6 timeouts, while the load average of the machine was above 500.sql.test.tsruns only the tests that need no docker.socket.test.ts,tcp-server.test.ts,tls-reject-before-client-cert.test.ts,node-tls-connect.test.ts,node-tls-server.test.ts,node-http2-upgrade.test.mts): 432 pass, 1 fail. The failure isshould not call drain before handshake, which needs DNS forwww.example.com. It fails the same way with the released bun.test/internal/source-lints/: 169 pass, 1 fail.primordials-exports.test.tsstopped at the 5 s timeout. It passes in 0.7 s when it runs alone.test/cli/create/create-jsx.test.ts: 5 pass, 8 fail, the same 8 with the released bun. They needui.shadcn.com, which this machine cannot resolve.bun createwith no argument (help text, exit 1),bun create oven-sh/<missing repository> ./out(the path throughfetch_all_local_and_remoteandExample::print),bun pm pack --help, andbun pm pack --destination --gzip-level(same tarball).cargo checkcovers the Rust code and CI covers the C++ code.Self-review
The self-review read a cut of 16 groups. I checked each of its claims in the PR diffs and in the code before I acted. The finishing edits in groups 1, 2 and 6 came after it. The compile checks, the probes and the tests above cover them. The 12 concerns:
allocatedparameter ofMimeType::init: node:fs: openAsBlob pins the file, and a read through the Blob verifies it #43896 adds a call with three arguments. git reports no conflict, and the build breaks after the second merge. Group removed.JSGlobalObjectSqlExt::sql_vmandArgumentsSlice::_vm: Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050 and socket, timer, sql, redis: replaceunsafewith typed ownership #40139 remove the field. Group removed.TransformOutstream: bun build --no-bundle: honor --sourcemap #41939 edits the same functions, and Remove dead code from the JSC, WebCore, and node bindings, uWS/uSockets, SecretsLinux, and seven Rust crates #42431 left it for a change that also handlesBundleOptions::output_dir_handle. Group removed.BOOL_FIELD_SETTERSentries: feat(mdx): add native compilation, loading, and direct serving #27047 reads the table with no filter, and its test setspermissiveAutolinks. Group removed.Bun__allocUint8ArrayForCopy: Remove dead code from react_compiler, the WebCore/IDL bindings, install_jsc, node-fallbacks, and misc crates #39420 kept it so thatArrayBuffer::allocmirrorsArrayBuffer::create. The removal also made aUint8Arraycall panic at run time. Group removed.help_command::Reason::InvalidCommand: the removal leaves an enum with one variant and a printer macro with one use. That is a refactor of the help printer. Group removed.Example::printkept anOptionthat no caller sets toNone. Finished in group 2.fetch_all_local_and_remoteandfetch_allkept a node parameter that no caller sets. Finished in group 2.is_container_compatiblekept a branch that the next branch covers. Finished in group 6.match templateininit_command.rshas an arm that cannot run. Rejected, see Fix.Subcommand::PackandJSCInlines.h. Added.Open PRs
PmSubcommandwith aPackvariant. It does not usebun_install'sSubcommand.backend_parameters. Each conflicts with this PR in that file, so git reports it.create_command.rs), sql(postgres): move the allocate-and-dial block of createConnection into PostgresSQLConnection::open #43942 (PostgresSQLConnection.rs), init: exit 1 when the nested bun install fails #38474 and Atomic writes: convert bun init and test a write that fails #39701 (init_command.rs). cli: remove unsafe from bun run / bun test / --filter and the other CLI commands #40242 and bun init: add react + @types/react to the blank scaffold so .tsx works #36607 conflict with main already. There is no conflict with socket, timer, sql, redis: replaceunsafewith typed ownership #40139, node:fs: openAsBlob pins the file, and a read through the Blob verifies it #43896, bun build --no-bundle: honor --sourcemap #41939, Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050 or feat(mdx): add native compilation, loading, and direct serving #27047.One deletion was wrong and is not in this PR
I first deleted
StoredTrace::from(src/bun_core/Global.rs).bun bdfailed:src/install/PackageInstall.rs:224calls it under#[cfg(bun_debug)]. Plaincargo checkand mordant do not set that cfg, so both see no caller. After the revert I searched the whole tree for every other removed name and compiled with each custom cfg.Found and left in place
allocatedparameter ofMimeType::initwithcontent_type_needs_freeinRequestContext.rs(node:fs: openAsBlob pins the file, and a read through the Blob verifies it #43896),TransformOutstreamwithBundleOptions::output_dir_handle(bun build --no-bundle: honor --sourcemap #41939),sql_vmand the VM parameter ofArgumentsSlice::init(Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050, socket, timer, sql, redis: replaceunsafewith typed ownership #40139), theformatpath ofpretty_format.rs(Remove dead code from the web streams bindings, the test runner formatter, and the bindgen codegen #41445),cssInternals.attrTestwithStyleAttribute(Remove dead code from bun_core, bun_jsc, bun_css, react_compiler, and two C++ bindings #40367),MaxHeapAllocator(Remove dead code from bun_jsc, bun_core, the bindgenv2 codegen, 15 error types, and the C++ bindings #40915).BOOL_FIELD_SETTERSentries thatparse_optionsinMarkdownObject.rsskips (feat(mdx): add native compilation, loading, and direct serving #27047).help_command::Reason::InvalidCommand. The secondmatch templateininit_command.rs.bun init --react tailwindcannot match because the argument must start with-.--dump-environment-variablesis parsed and ignored.publishalways sendsworkspaces/false.CharacterSettable, the Postgres binaryint2,oidandtimetzparsers, the MySQLTimeencode path.httplog!macro insrc/runtime/server/mod.rsbuilds its arguments and prints nothing.server_body.rshas a working one.Request::detach_lifetimeinpicohttp,WebSocket::clear_send_buffers(free),InternalState::content_encoding_i.no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check